Hold in-flight monitor updates until background event processing#4377
Hold in-flight monitor updates until background event processing#4377wpaulino wants to merge 3 commits intolightningdevkit:mainfrom
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4377 +/- ##
==========================================
- Coverage 86.01% 86.01% -0.01%
==========================================
Files 156 156
Lines 102857 102879 +22
Branches 102857 102879 +22
==========================================
+ Hits 88476 88492 +16
- Misses 11871 11875 +4
- Partials 2510 2512 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lightning/src/ln/channelmanager.rs
Outdated
| for event in background_events.drain(..) { | ||
| match event { | ||
| BackgroundEvent::MonitorUpdateRegeneratedOnStartup { counterparty_node_id, funding_txo, channel_id, update } => { | ||
| // We previously assumed background events would eventually be processed prior |
There was a problem hiding this comment.
Are you sure we need this change in this case? The whole point of RegeneratedOnStartup is that we don't care if we lose it cause it'll be regenerated when we restart.
There was a problem hiding this comment.
Yeah I misinterpreted how this specific case works.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
wpaulino
force-pushed
the
hold-in-flight-updates-until-background-event-processing
branch
from
2a15c45 to
c880d85
Compare
wpaulino
force-pushed
the
hold-in-flight-updates-until-background-event-processing
branch
from
c880d85 to
55d9ef0
Compare
wpaulino
force-pushed
the
hold-in-flight-updates-until-background-event-processing
branch
from
55d9ef0 to
398e624
Compare
|
✅ Added second reviewer: @joostjager |
|
✅ Added second reviewer: @valentinewallace |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
Makes sense. I think we can drop some code but I think it works as-is
| highest_update_id_completed: max_in_flight_update_id, | ||
| }); | ||
| } else { | ||
| $chan_in_flight_upds.retain(|update| { |
There was a problem hiding this comment.
I do find these updates a bit subtle. It might be easier to reason about if we symmetrically didn't remove any in_flight_updates in both cases, not sure though.
There was a problem hiding this comment.
That might require reworking the MonitorRegeneratedOnStartup case and I'd rather keep these changes minimally focused on the bug fix for now.
We previously assumed background events would eventually be processed prior to another `ChannelManager` write, so we would immediately remove all in-flight monitor updates that completed since the last `ChannelManager` serialization. This isn't always the case, so we now keep them all around until we're ready to handle them, i.e., when `process_background_events` is called. This was discovered while fuzzing `chanmon_consistency_target` on the main branch with some changes that allow it to connect blocks. It was triggered by reloading the `ChannelManager` after a monitor update completion for an outgoing HTLC, calling `ChannelManager::best_block_updated`, and reloading the `ChannelManager` once again. A test is included that provides a minimal reproduction of this case.
wpaulino
dismissed stale reviews from valentinewallace and TheBlueMatt
via
f128b85
wpaulino
force-pushed
the
hold-in-flight-updates-until-background-event-processing
branch
from
398e624 to
f128b85
Compare
4 participants
We previously assumed background events would eventually be processed prior to another
ChannelManagerwrite, so we would immediately remove all in-flight monitor updates that completed since the lastChannelManagerserialization. This isn't always the case, so we now keep them all around until we're ready to handle them, i.e., whenprocess_background_eventsis called.This was discovered while fuzzing
chanmon_consistency_targeton the main branch with some changes that allow it to connect blocks. It was triggered by reloading theChannelManagerafter a monitor update completion for an outgoing HTLC, callingChannelManager::best_block_updated, and reloading theChannelManageronce again. A test is included that provides a minimal reproduction of this case.